Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed whitespace handling for partials #212

Merged
merged 5 commits into from
Mar 30, 2021

Conversation

dpoetzsch
Copy link
Contributor

As mentioned in #211 the handling of whitespace for partials of mustache.java is not conforming to the mustache spec.

This PR fixes this (only for partials though).

Basic idea is to memorize the indentation of a partial and to pass it through during execution via a custom writer.

This is not yet finished, as this obviously broke some other tests (as the whitespace differs). I just wanted to know if this is at all a direction that would be considered for merging.

@spullara
Copy link
Owner

This seems reasonable. I may not want to make it the default behavior as it isn't backwards compatible with the current whitespace handling but I'm open to that conversation as well. Thanks.

@spullara
Copy link
Owner

spullara commented Sep 11, 2018

Took a look at the code. I think that this could be better done during compilation rather than at runtime. The current version will definitely affect performance by a lot. Since we know the indentation at parse time we could probably modify the text found in the partial at the same time to adjust the indentation.

The only odd situation would be recursion...

@dpoetzsch
Copy link
Contributor Author

dpoetzsch commented Sep 12, 2018

I also thought about the performance problem.

From what I see, for non-recursive templates we could do this at compilation but will create a larger cache / compiled data structure as we would need a different partial object for each indentation. E.g. if a partial is used at 4 different indentation levels we would need to create 4 different variants of it.

For recursive templates I don't see a way around doing this at runtime. An example why this could be useful even for recursive templates:

{{#items}}
- {{t}}
  {{> p}}
{{/items}}

Called with

items: [
  { t:"i1", items: [
    { t: "i1.1", items: [
      {t: "i1.1.1", items:[]}
    ] },
    { t: "i1.2", items: [] }
] }

should result in

- i1
  - i1.1
    - i1.1.1
  - i1.2

I could not find a spec case for indentation and whitespace. However the ruby implementation which seems to be quite "official" actually yields the expected outcome.

So the question is if it is worth the extra complication and memory footprint to have a compile-time solution for non-recursive partials. One could have a switch that enables whitespace-conformity, allowing for faster rendering if whitespace is not an issue or slower rendering if it is. For backwards compatibility the switch could be off by default. If needed, the switch could even be implemented at compile time, by using different variants of PartialCode at compile time depending on the chosen strategy.

@spullara
Copy link
Owner

I've known about the whitespace issue for a long time, it hasn't been implemented because of the performance issues with it and almost all of the generated text isn't sensitive to it (HTML). Let me see if there is a way to implement it with some overrides so that if people want this behavior I could include the ability to do it without changing the normal high performance behavior.

@dpoetzsch
Copy link
Contributor Author

I took the time to update my changes so that by default nothing changes and performance is not impacted.

For people that want whitespace according to the specs I added another SpecMustacheFactory that overrides the relevant methods.

To make review easier, I handle only the (most problematic) case of partials in this PR. See #215 for a solution that fixes whitespace for all spec cases.

@dansimpson
Copy link

I'd like to see this merged and published. Using the SpecMustacheFactory seems like a nice opt-in solution here for those of us using mustache for whitespace significant output formats.

@alexchow
Copy link

alexchow commented Apr 2, 2020

Will this get merged?

@DavidSzalai
Copy link

We'd need it too, generating yaml spec with mustache, where indentation counts.

@spullara spullara merged commit 8fabfcd into spullara:master Mar 30, 2021
@agentgt
Copy link

agentgt commented Nov 10, 2022

I assume this still hasn't been released yet?

I have a mustache library (https://github.com/jstachio/jstachio) that so far has achieved mostly 1:1 with JMustache as both my library and JMustache implement the whitespace but I cannot with mustache.java because of the whitespace issue (as well as lambda #286 ).

Ideally I was hoping for duplicate output to reuse tests as well as make an option for people using JStachio to safely switch to mustache.java if runtime templates are needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants